This should stay

This was moved into each sub-command, so still there in a way.

Why making these flags per stack subcommand instead of putting the on the stack command and make the subcommand inherit ?

Because stack ls now needs a different behaviour than other stack sub-commands for --namespace and we cannot set it once with type string and another time with []string (e.g. several namespaces).
So the drawback is a bit of code duplication setting the flags for all sub-commands, which was factorized into kubernetes.InstallFlags

Will setting a default here will override the default namespace defined in the context ?

Hmm yes, I suppose it will, which is likely a bug.
However I don't think this has been introduced by this PR as the default for --namespace was already default.
Could this be fixed in a follow up?

Actually there is code in kubernetes.WrapCli to deal with that and it seems to be working as expected

> docker stack ls  --orchestrator=kubernetes
stacks.compose.docker.com is forbidden: User "uu" cannot list stacks.compose.docker.com in the namespace "default": access denied
> kubectl config set-context ucp_34.209.72.126:6443_uu --namespace=tutu
Context "ucp_34.209.72.126:6443_uu" modified.
> docker stack ls  --orchestrator=kubernetes
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
dtc                 5                   Kubernetes          tutu

all-namespaces should probably be marked as experimentalCLI too

Yes, I'll add that, thanks.

sort.Slice(stacks, func(i, j int) bool { …

OK

Same here

and here …

… and here

We may want to share this error somehow

OK!

I don't get this change, this feels really weird to add the flag twice (on cmd.Flags() and cmd.PersistentFlags())

"orchestrator" is the only flag added to cmd.PersistentFlags() in InstallFlags, all the other flags are still in cmd.Flags()

That being said if you would rather have me remove that last commit, I'd be fine with that!

Slightly confused as well

OK, I'll just remove the related commit.

Looks like (default swarm) no longer matches reality (for example, having "orchestrator":"kubernetes" in my ~/.docker/config.json would set the default)

That's a good point. I'll remove the (default swarm).

Also this was there before but I don't thing with the docker cli brings anything and the other flag descriptions don't have it. I'll remove it too.

Looks like this change to all-namespaces should be done in the other PR (#991)?

You're right, too late though as #991 has been merged in the meanwhile…

Is there a reason we use two separate approaches for the namespace flag? Looks like we define it twice now: once through AddNamespaceFlag()

All docker stack sub-commands use AddNamespaceFlag and have a namespace flag defined as a StringVar except ls which has a namespace flag defined as a StringSliceVar.
This is because for all sub-commands but ls only one --namespace can be set whereas ls can have several, e.g. docker stack ls --namespace=tutu,default,bla or docker stack ls --namespace=tutu --namespace=default --namespace=bla.

Wondering now if this should be a StringSliceVar or a StringVar; I think StringVar can be specified multiple times (--namespace=bla --namespace=foo), but does not support comma-separated values; we haven't documented using comma-separated values, and don't think we support it for other options (possibly --security-opt only)

(note that if a command only accepts a single namespace, we could still produce an error if multiple are given)

I'll take another look but from what I remember there was no way to make it look nice, for both cases, e.g.

$ docker stack ps  --help
…
Options:
…
      --namespace string    Kubernetes namespace to use

VS

$ ./build/docker.exe stack ls --help
…
Options:
…
      --namespace strings   Kubernetes namespaces to use

Also I don't see how a single StringVar can hold multiple --namespace values…

I think StringVar can be specified multiple times (--namespace=bla --namespace=foo), but does not support comma-separated values

In that case the latest value is silently selected (here foo), I suppose the flags are simply processed in order.
If we just let comma-separated values out of the question we could indeed emulate both behaviors with a StringSliceVar by using the last element when only one namespace is required.

(Incidentally passing --namespace=bla,foo to a StringVar flag yields a value of bla,foo…)

The best I could come up with by using []string on the stack command level is

$ ./build/docker.exe stack --help

Usage:  docker stack COMMAND

Manage Docker stacks

Options:
      --kubeconfig string   Kubernetes config file
      --namespace strings   Kubernetes namespace to use

This is wrong for all sub-commands as for most of them it says strings plural whereas only one is supported (and differs for instance from kubeconfig), and for docker stack ls which supports several namespaces the description should have namespaces…

I don't see how to have types and descriptions accurate for all sub-commands without duplicating the flag.

Looks like this is to get rid of duplicates? This feels a bit like premature optimisation, also I'd expected this to return a []string (matching the type before removing duplicates)

Yes this is to remove duplicates, again to match kubectl behavior. I'll change the return type to a []string.

…and refactor it to

func removeDuplicates(namespaces []string) []string {
	mnms := map[string]struct{}{}
	for _, nm := range namespaces {
		mnms[nm] = struct{}{}
	}
	namespaces = []string{}
	for nm := range mnms {
		namespaces = append(namespaces, nm)
	}
	return namespaces
}

There's now opts.Namespaces (for stacks) and opts.Namespace, correct?

Yes, there first one is of type []string and the latter string.

Wouldn't len(opts.Namespaces) give the same here?

Oh yes, nice catch!

This could just take (flags *flag.FlagSet, or even namespaces []string, then call it with getNamespaces(cmd.Flags()), but see my other comments

This has actually been changed to receive a flags *flag.FlagSet in a follow-up PR, but I might as well do it there.

Hmm sorry I meant []string and passing it opts.Namespaces…

Both flags and namespace basically come from cmd.Flags(); why pass both, if we have all information in flags?

The way Kubernetes works implies that listing the stacks (or anything else) must be made within the context of a namespace, thus as we want to merge the lists of stacks from different namespaces we create a new kubernetes.Options for each namespace when iterating over them for docker stack ls.
I'll refactor the code to separate the different behaviors in respect to namespaces.

in what situation would flags.GetString("namespace") give anything different than namespace[0]? If we only expect a single namespace to be passed, it should not take more than one

This is again due to the discrepancy between docker stack ls and the other docker stack sub-commands regarding the number of --namespace flags allowed.

Slightly confused as well

So if you have orchestrator=all and then you do docker stack ls --namespace=foobar, this will still return all stacks in every namespace?

This branch is only executed if opts.AllNamespaces || len(opts.Namespaces) == 0 e.g. there is already --orchestrator=all or no --namespace.
Here is what this gives me:

$ ./build/docker.exe --orchestrator=all stack ls
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
blaaaaaa            1                   Kubernetes          default
blaaaaaaaaa         1                   Kubernetes          default
dtc                 5                   Kubernetes          tutu
dtc                 5                   Kubernetes          tata
dtcccccc            5                   Kubernetes          default

$ ./build/docker.exe --orchestrator=all stack ls --namespace=tata
NAME                SERVICES            ORCHESTRATOR        NAMESPACE
dtc                 5                   Kubernetes          tata

Ah, I see now.

Ah, I see now.

This is redundant, because we already checked HasKubernetes(), which also checks for HasAll();

// HasKubernetes checks if kubernetes orchestrator is enabled
func (c ClientInfo) HasKubernetes() bool {
	return c.HasExperimental && (c.Orchestrator == OrchestratorKubernetes || c.Orchestrator == OrchestratorAll)
}

--orchestrator=kubernetes yields HasKubernetes() true and HasAll() false
--orchestrator=all yields both HasKubernetes() true and HasAll() true
So if HasAll() implies HasKubernetes(), the reverse is not necessary true.

But here HasAll() (ie --orchestrator=all) is used to enable --all-namespaces; I don't see how those relate

This is a UX requirement defined in the PRD

• Provide intuitive mechanisms for switching Kubernetes namespace:
  • When targeting all orchestrators, target all namespaces by default

I'm having trouble conflating these; this may become very confusing:

• configuration has "orchestrator: all": docker stack ls shows all stacks (both swarm and k8s)
• I want to view only the k8s stacks, so I add --orchestrator=kubernetes. Now I no longer see all my stacks, only those in the default namespace
• confused

The thinking behind this was as follows: When you list stacks across orchestrators your intention is to see all the stacks running on your cluster. When you specify kubernetes as your orchestrator; you are doing something more specific and want similar behaviour to what kubectl gives you.

I can see how this might be confusing for your flow. There will be a NAMESPACE column which should help the user understand what has happened.

Ah, see you're referring to the case when a user has set orchestrator to all in their config file. I don't think many users will do this as they will only be able to list resources. In order to manipulate resources, they will need to specify an orchestrator.

no need to loop twice here, could be something like;

func removeDuplicates(namespaces []string) []string {
	found := make(map[string]bool)
	results := namespaces[:0]
	for _, n := range namespaces {
		if !found[n] {
			results = append(results, n)
			found[n] = true
		}
	}
	return results
}

Fair enough, I'll change that.

Wondering now if this should be a StringSliceVar or a StringVar; I think StringVar can be specified multiple times (--namespace=bla --namespace=foo), but does not support comma-separated values; we haven't documented using comma-separated values, and don't think we support it for other options (possibly --security-opt only)

(note that if a command only accepts a single namespace, we could still produce an error if multiple are given)

Thinking of rephrasing this (also in light of #770); perhaps;

No orchestrator specified. Use either "kubernetes" or "swarm"

(better suggestions welcome)

As an error message shouldn't it start with a lower case? I would probably avoid the dot too, e.g.

no orchestrator specified, use either "kubernetes" or "swarm"

or

no orchestrator specified: use either "kubernetes" or "swarm"

?